Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix HTTP2StreamChannel leak #657

Merged
merged 3 commits into from
Feb 14, 2023
Merged

Fix HTTP2StreamChannel leak #657

merged 3 commits into from
Feb 14, 2023

Conversation

fabianfett
Copy link
Member

@fabianfett fabianfett commented Jan 24, 2023 •
edited
Loading

We leak HTTP2StreamChannels in long living HTTPClients. The reason for this is that we registered the callback to remove an HTTP2StreamChannel from the openStreams set in HTTP2Connection on the HTTP2 connection channel and not on the HTTP2StreamChannel itself. (Side note: self.channel vs local var channel is always crazy dangerous)

Changes

  • Actual bug fix (remove a self.)
  • Ensuring that we always close the HTTP2StreamChannel when the request has finished
  • Add a test for a previous uncovered code path (request receives full response before being done writing)

@fabianfett fabianfett added the semver/patch For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Jan 24, 2023
fabianfett
fabianfett commented Jan 24, 2023
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift
@@ -241,7 +241,7 @@ final class HTTP2Connection {
// before.
let box = ChannelBox(channel)
self.openStreams.insert(box)
self.channel.closeFuture.whenComplete { _ in
channel.closeFuture.whenComplete { _ in
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual bug fix. However I have no idea how to test it!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have run into this just today. If we send a too large header and the server sends a go away frame the child channel is closed but the parent channel not. This might be a good way to test this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can try to test this by triggering the shutdown event and finding which channels get sent this user event. So long as we can hook the parent channel we should observe that it doesn't see it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is: How do I get access to the stream/child channel? All the ideas I had so far a quite locked down. Of course I can add an internal function that gives me the currently active channels.

func forTestingGetActiveChildChannels -> EventLoopFuture<[Channel]>

wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a testing function is fine. You can guard it behind an SPI, which is probably sufficient.

@fabianfett fabianfett force-pushed the ff-fix-HTTP2StreamChannel-leak branch from b8e3bad to bfb149c Compare January 24, 2023 16:54
Lukasa
Lukasa reviewed Jan 24, 2023
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift Outdated
promise.futureResult.whenComplete { _ in
// Once we have written the last message we must close the http2 stream, since this
// break a reference cycle in HTTP2Connection.
context.close(promise: nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little surprised that this is necessary. swift-nio-http2 should notify you that the stream is closed when both sides have sent their .end. Are you entirely confident this is required?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not. I was only testing the child channel. And wanted to be as defensive as possible. Do you want me to remove it again?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we shouldn't add code that we don't really need.

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift
@@ -241,7 +241,7 @@ final class HTTP2Connection {
// before.
let box = ChannelBox(channel)
self.openStreams.insert(box)
self.channel.closeFuture.whenComplete { _ in
channel.closeFuture.whenComplete { _ in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can try to test this by triggering the shutdown event and finding which channels get sent this user event. So long as we can hook the parent channel we should observe that it doesn't see it.

@dnadoba dnadoba mentioned this pull request Jan 25, 2023
Merged
@fabianfett fabianfett force-pushed the ff-fix-HTTP2StreamChannel-leak branch 3 times, most recently from 03f4234 to 0ddba75 Compare February 7, 2023 16:27
glbrntt
glbrntt approved these changes Feb 7, 2023
View reviewed changes
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good modulo a couple of nits.

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift Show resolved Hide resolved
Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift Show resolved Hide resolved
@fabianfett fabianfett force-pushed the ff-fix-HTTP2StreamChannel-leak branch from 0ddba75 to 707eef1 Compare February 7, 2023 17:00
@fabianfett
Copy link
Member Author

@glbrntt added/updated code comments to point out the http/1.1 semantics of the action

@dnadoba dnadoba merged commit e264599 into swift-server:main Feb 14, 2023
@dnadoba dnadoba deleted the ff-fix-HTTP2StreamChannel-leak branch February 14, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lukasa Lukasa Lukasa left review comments

@dnadoba dnadoba dnadoba approved these changes

@glbrntt glbrntt glbrntt approved these changes

Assignees
No one assigned
Labels
semver/patch For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fabianfett @Lukasa @glbrntt @dnadoba